Skip to content

Refactor authentication logic in fetch.ts #1415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Refactor authentication logic in fetch.ts #1415

wants to merge 1 commit into from

Conversation

kausikds
Copy link

@kausikds kausikds commented May 19, 2025 •
edited
Loading

What kind of change does this PR introduce?

  • 🛠 Refactor (Code improvement without changing functionality)

What is the current behavior?

  • fetch.ts currently sets accessToken using a nullish coalescing (??) fallback.

  • This can lead to unintended behavior where supabaseKey is assigned if getAccessToken() returns undefined or null.

Before (Current behavior):

const accessToken = (await getAccessToken()) ?? supabaseKey;

if (!headers.has('Authorization')) {
      headers.set('Authorization', `Bearer ${accessToken}`)
    }

What is the new behavior?

  • The refactored code separates token retrieval from fallback logic.

  • Now, authentication headers are set only if accessToken exists.

  • This improves clarity and avoids accidental assignment of Authorization: Bearer undefined.

After (Refactored behavior):

const accessToken = await getAccessToken()

if (accessToken && !headers.has('Authorization')) {
      headers.set('Authorization', `Bearer ${accessToken}`)
    }

Additional context

  • This change improves readability without altering core functionality.

  • No breaking changes expected.

  • Code remains compatible with the existing authentication flow.

@kausikds
Copy link
Author

Hello @grdsdev , could you please review the pull request?

@j4w8n
Copy link
Contributor

j4w8n commented May 26, 2025

supabaseKey needs to be the fallback here. If something goes wrong elsewhere, it ensures any requests to supabase are at least using the anon key.

@mandarini mandarini self-assigned this Jul 24, 2025
@mandarini
Copy link
Contributor

Hi @kausikds! Thank you for taking the time to contribute to Supabase and for thinking about code clarity. I appreciate your attention to detail!

However, I need to close this PR because the current implementation is actually working as intended. Let me explain why the fallback to supabaseKey is essential. Supabase is designed to work in two modes:

  1. Authenticated requests: When a user is logged in, we use their JWT token
  2. Anonymous requests: When no user is logged in, we use the supabaseKey (typically the anon key)

The line const accessToken = (await getAccessToken()) ?? supabaseKey ensures that

• Public resources remain accessible - Many Supabase projects have public tables/functions that should work without user authentication
• Row Level Security (RLS) policies work correctly - RLS policies can be designed to handle both authenticated users and anonymous access
• Service keys function properly - Server-side usage often relies on service keys when no user session exists
• API requests always have authorization - The Supabase API expects an Authorization header in all requests

If we removed the fallback (?? supabaseKey):
• Anonymous users couldn't access public resources
• Requests would fail with authentication errors when they should succeed
• Server-side applications would break when no user session exists

Consider a blog app where:
• Logged-in users can see private posts (uses JWT token)
• Anonymous visitors can see public posts (uses anon key)

The proposed change would prevent anonymous visitors from seeing public posts, breaking the intended functionality.

Thanks again for the contribution! If you have other ideas for improvements, please don't hesitate to open discussions or issues, and tag me! I'll be happy to take a look!

@mandarini mandarini closed this Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mandarini mandarini

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kausikds @j4w8n @mandarini